Refactor/standalone tests #249
Conversation
| @@ -2,7 +2,7 @@ use super::ROOT_PATH; | |||
| use crate::service::utils::make_request; | |||
| use anyhow::Result; | |||
|
|
|||
| #[tokio::test] | |||
| #[tokio_shared_rt::test(shared)] | |||
There was a problem hiding this comment.
What are the benefits for service tests of using the shared tokio runtime?
There was a problem hiding this comment.
Not using it can cause conflict when using the connectors.
There was a problem hiding this comment.
It's looking good! Now that you are at it, could you also improve .github/workflows/test.yml to eliminate the steps that are not needed anymore? (building and running the service in the background, syncing the neo4j data, waiting for it to be available, etc).
| let should_reindex = redis_is_empty().await.unwrap_or(false); | ||
| if should_reindex { | ||
| info!("Starting reindexing process."); | ||
| reindex().await; | ||
| } |
There was a problem hiding this comment.
This logic is not needed. We can do reindex().await
|
|
||
| // Run the run-queries.sh script on the Docker host using docker exec | ||
| tokio::process::Command::new("docker") | ||
| .args(&["exec", "neo4j", "bash", "/db-graph/run-queries.sh"]) |
There was a problem hiding this comment.
There is a few trade offs with this approach as:
- We don't know where neo4j is (maybe not docker, maybe different containter name (as in
pubky-docker) - Very long waits when you simply want to run a single test case often during development
Is there a way we can run only sync_graph() optionally? I am thinking maybe we can have a more granular control envvar than SYNC_DB, maybe an explicit SYNC_GRAPH variable ? We already know whether to reindex or not with the REINDEX envvar, this way we control both independently.
There was a problem hiding this comment.
Will see what I can do.
About your first point, that's very true. It was just a ton easier to do it this way for now. After all, it's for local tests and pipelines which won't be changing often.
| setup(&config).await; | ||
|
|
||
| // make sure DBs are in sync with mock data | ||
| let sync_db_env = std::env::var("SYNC_DB").unwrap_or("false".to_string()); |
There was a problem hiding this comment.
Reminder to add the new envvars to .env-sample with documentation of that they do
There was a problem hiding this comment.
I'll fix the docs. This doesn't necessarily need to be in .env-sample after it's mentioned in the how to run the tests parts of the README.
#172
Pre-submission Checklist
cargo test.cargo bench